Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor golangci-lint config and remove redundant nolints #1486

Merged
merged 2 commits into from
Feb 11, 2023
Merged

Refactor golangci-lint config and remove redundant nolints #1486

merged 2 commits into from
Feb 11, 2023

Conversation

alexandear
Copy link
Contributor

@alexandear alexandear commented Feb 9, 2023 •
edited
Loading

This PR does the following:

  • add additional linter run options in GitHub lint's workflow;
  • remove unused //nolint comments;
  • use golangci-lint-action because it's shorted and caches data.

Options:

  • --verbose for clear visibility of which set of linters is run;
  • --enable=gochecknoinits enables gochecknoinits (the repo has one init() which already nolinted);
  • --enable=nolintlint enables linter that reports redundant //nolint comments.

structcheck linter is deprecated and automatically disabled by golangci-lint.

@alexandear alexandear changed the title Refactor golangci-lint config Refactor golangci-lint config and remove redundant nolints Feb 9, 2023
@alexandear
Refactor golangci-lint config
468b2c8 
- Use golangci-lint-action for GitHub workflow.
- Add additional golangci-lint run options.
- Remove unused nolint directives.
@erikdubbelboer
Copy link
Collaborator

I'm sorry but I don't agree with removing nolint:errcheck. I know that happens a lot in tests. But I would still prefer to mark each line individually as it makes the reader aware that an error is ignored here. Without it it's hard to see if this happens or not. I also wouldn't want any new code to be added later where we forget to check there error and the linter doesn't complain.

@alexandear
Copy link
Contributor Author

alexandear commented Feb 10, 2023 •
edited
Loading

I'm sorry but I don't agree with removing nolint:errcheck. I know that happens a lot in tests. But I would still prefer to mark each line individually as it makes the reader aware that an error is ignored here. Without it it's hard to see if this happens or not. I also wouldn't want any new code to be added later where we forget to check there error and the linter doesn't complain.

@erikdubbelboer Thanks for the comment. Partially agree. Could you comment on the changed code lines? Because I don't understand what should I do. Reset only //nolint:errcheck changes?

@alexandear
Copy link
Contributor Author

Maybe we should rewrite tc.Close() \\nolint:errcheckto _ = tc.Close()? This states explicitly we ignore the returned error.

@erikdubbelboer
Copy link
Collaborator

Maybe we should rewrite tc.Close() \\nolint:errcheckto _ = tc.Close()? This states explicitly we ignore the returned error.

Yes, please remove --exclude-use-default as it disables a lot more checks than just errcheck. And then just add _ = to lines that fail on errcheck.

@alexandear
Copy link
Contributor Author

@erikdubbelboer done

@erikdubbelboer erikdubbelboer merged commit 934f04e into valyala:master Feb 11, 2023
@erikdubbelboer
Copy link
Collaborator

Thanks!

@alexandear alexandear deleted the refactor-linter-config branch February 11, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexandear @erikdubbelboer